Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

impr(profiling): increase buffer length from 10s to 60s #4826

Merged
merged 8 commits into from
Feb 13, 2025

Conversation

armcknight
Copy link
Member

We need to increase the continuous profiling client buffer size to 60 seconds (from https://www.notion.so/sentry/Continuous-UI-Profiling-SDK-API-Spec-17e8b10e4b5d80c59a40c6e114470934?pvs=4)

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 9.108%. Comparing base (18bf9a5) to head (141ad1b).
Report is 6 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (18bf9a5) and HEAD (141ad1b). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (18bf9a5) HEAD (141ad1b)
4 1
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main    #4826        +/-   ##
=============================================
- Coverage   91.519%   9.108%   -82.412%     
=============================================
  Files          655      350       -305     
  Lines        76553    24934     -51619     
  Branches     27683       94     -27589     
=============================================
- Hits         70061     2271     -67790     
- Misses        6393    22663     +16270     
+ Partials        99        0        -99     

see 650 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18bf9a5...141ad1b. Read the comment docs.

Copy link

github-actions bot commented Feb 10, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1216.27 ms 1237.35 ms 21.08 ms
Size 22.31 KiB 819.53 KiB 797.22 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
734d507 1201.78 ms 1213.96 ms 12.18 ms
3d8532d 1225.27 ms 1238.80 ms 13.53 ms
46c6025 1213.87 ms 1253.06 ms 39.19 ms
caa37b6 1221.19 ms 1238.71 ms 17.52 ms
80f2f39 1223.35 ms 1247.16 ms 23.80 ms
a5c946b 1219.33 ms 1236.53 ms 17.20 ms
a366f3b 1230.65 ms 1251.88 ms 21.23 ms
b3fd863 1217.81 ms 1231.31 ms 13.50 ms
5a6e387 1217.62 ms 1240.38 ms 22.76 ms
8636ef0 1239.96 ms 1253.14 ms 13.18 ms

App size

Revision Plain With Sentry Diff
734d507 22.85 KiB 412.66 KiB 389.81 KiB
3d8532d 22.85 KiB 414.09 KiB 391.24 KiB
46c6025 22.85 KiB 408.84 KiB 385.99 KiB
caa37b6 21.58 KiB 424.34 KiB 402.76 KiB
80f2f39 21.58 KiB 704.94 KiB 683.35 KiB
a5c946b 20.76 KiB 431.93 KiB 411.17 KiB
a366f3b 21.58 KiB 614.73 KiB 593.15 KiB
b3fd863 21.58 KiB 706.85 KiB 685.27 KiB
5a6e387 21.58 KiB 706.06 KiB 684.48 KiB
8636ef0 22.84 KiB 403.18 KiB 380.34 KiB

Previous results on branch: armcknight/impr(profiling)/increase-buffer

Startup times

Revision Plain With Sentry Diff
bf8d5b4 1230.32 ms 1246.85 ms 16.53 ms
3e8dfd8 1244.21 ms 1257.37 ms 13.16 ms
8b7bf92 1224.16 ms 1245.69 ms 21.53 ms
d76b80f 1237.73 ms 1256.94 ms 19.20 ms

App size

Revision Plain With Sentry Diff
bf8d5b4 22.31 KiB 817.40 KiB 795.08 KiB
3e8dfd8 22.31 KiB 817.40 KiB 795.09 KiB
8b7bf92 22.31 KiB 818.13 KiB 795.82 KiB
d76b80f 22.31 KiB 814.96 KiB 792.65 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member Author

@armcknight armcknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philipphofmann I wound up needing to perform a substantial refactoring of TestSentryNSTimerFactory to add a new function, which led to finding some weirdness in other test cases, and iterating through this a few times. Please check out my comments below.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tests, LGTM

@armcknight armcknight merged commit d3a65fb into main Feb 13, 2025
59 of 68 checks passed
@armcknight armcknight deleted the armcknight/impr(profiling)/increase-buffer branch February 13, 2025 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants